-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 44: Approximate inference vignette #69
Conversation
Updates based on #70:
|
f9d6cf2
to
c799f97
Compare
@kgostic @seabbs @parksw3 could you give some midway feedback before I finish up this first version of the approximate inference vignette? My view of what needs to be done before a first merge:
My view of what can be left to a second try at improving this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content so far looks okay. I assume its an initialisation issue for pathfinder. have you given multipathfinder a spin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content so far looks okay. I assume its an initialisation issue for pathfinder. have you given multipathfinder a spin?
Thanks for the suggestion Sam, haven't tried it yet (though the algorithm did have four runs, and perhaps 1-2 of them had this error, so I may already have been using the "multi" version). To try r.e. fixing pathfinder (open to other suggestions!):
|
5640135
to
8f2be56
Compare
Steps needed to merge this are:
|
Do you actually want a review given outstanding tasks and failing CI or advice on something or? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice. A few typo suggestions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
===========================================
+ Coverage 28.49% 38.69% +10.20%
===========================================
Files 10 10
Lines 565 553 -12
===========================================
+ Hits 161 214 +53
+ Misses 404 339 -65 ☔ View full report in Codecov by Sentry. |
Hi @athowes thanks for writing this vignette. Really insightful and similar to what I'm writing for EpiNow2 here epiforecasts/EpiNow2#695. I'd be happy for you to review when it's ready and if you're interested. Two comments:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small typo noted but other than that this is looking good to me!
Thanks so much for reading @jamesmbaazam! I've added the units, and included your suggestion about moving some of the technical detail to an appendix in the follow-up issue about this vignette (#141). |
This looks amazing. I'm shocked at how fast other approximate methods run and how well they work... I found one typo.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice now. Good to see all the love! I agree with the idea of moving some technical terms to an appendix.
Description
This PR closes #44.
Extensions moved to issue: #141
Checklist